-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for overrideAttrs (split from #201734) #211685
Conversation
(cherry picked from commit 43c8b43)
The report looks great! The relevant numbers are all <1%. (And ignore cpuTime as always, although I wish -10% was true) If you could add some tests in |
@@ -16,17 +16,22 @@ let | |||
# This function introduces `overridePythonAttrs` and it overrides the call to `buildPythonPackage`. | |||
makeOverridablePythonPackage = f: origArgs: | |||
let | |||
ff = f origArgs; | |||
overrideWith = newArgs: origArgs // (if pkgs.lib.isFunction newArgs then newArgs origArgs else newArgs); | |||
args = lib.fix (lib.extends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By inlining fix
and extends
, and then simplifying the expression, you'll improve performance, but the gain will be marginal, as the performance stats indicate that this addition is in fact quite cheap. Readability is more important for now, as you're still working on this code.
5eb8237
to
d3db1cc
Compare
// shouldn't be used when overrideAttrs is available here we can use extends instead of overrideAttrs for performance
only outputs the first failing test atm
d3db1cc
to
9c0ac56
Compare
The effect of `.out // { outputSpecified = false; }` in these cases is to select the default output explicitly, but then make the selection implicit until `overrideAttrs` is called. Previously `overrideAttrs` would not preserve output selection, masking the apparently unnecessary behavior of this workaround. For `libllvm-polly`, this logic does not apply, as it does not select the default output. The `outputSpecified` workaround was introduced in NixOS#122554 and was perhaps rushed because of a release deadline, and expected delays from mass rebuilds. The change in `overrideAttrs` behavior was added in 5b2f597 / NixOS#211685 and the problem was discovered in NixOS#218537, which may contain further information.
The effect of `.out // { outputSpecified = false; }` in these cases is to select the default output explicitly, but then make the selection implicit until `overrideAttrs` is called. Previously `overrideAttrs` would not preserve output selection, masking the apparently unnecessary behavior of this workaround. For `libllvm-polly`, this logic does not apply, as it does not select the default output. The `outputSpecified` workaround was introduced in #122554 and was perhaps rushed because of a release deadline, and expected delays from mass rebuilds. The change in `overrideAttrs` behavior was added in 5b2f597 / #211685 and the problem was discovered in #218537, which may contain further information.
Description of changes
Fixes from #201734 that don't come with a significant performance impact.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes